CU-8699u4p99: Explicitly add elasticsearch dependency. #23
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also allow later eland versions.
|
@vladd-bit Do you think ES8 or ES9 dependency makes sense for this? |
|
yeah it def does. maybe it would be good to use the native lib instead of relying on eland for the ES query logic and so on, and just have some wrappers. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Also allow later
elandversions.The previous
requirements.txtdidn't specifyelasticsearchas a dependency. Though parts of the project do use and require it.At the same time, an
elandrequirement was specified, which mean a compatibleelasticsearchversion was available.This was most likely because some time ago (before Feburary 20 2023; medct v1.7.0)
medcatitself declared anelasticsearchdependency and working_with_cogstack was probably trying to work with the version that the library supported.This PR explicitly states the
elasticsearchdependency.Though I've allowed for a more wide range than before, on both
elasticsearchandeland.This is (currently) set to ES8 because (as far as I know) some sites still use ES8 (e.g KCH), while other sites use ES9 (GSTT).
PS:
I am not fully sure whether this setup would work for both ES8 and ES9. But I've left it at v8 for now.
PPS:
Currently, none of the ES-specific stuff is automatically tested (as far as I know). So it's hard to tell what does and doesn't work. Adding automated tests for this is on the roadmap, but is waiting on a few other things to happen first.